Squiz/ClassFileName: various improvements #845
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
11 tasks
braindawg
approved these changes
Mar 7, 2025
This commit adds some extra tests for the sniff. * Document handling of unfinished OO declarations (which contain enough info to still action). * Safeguard that comments in unexpected places doesn't cause problems. As this sniff looks at the file name of the file containing the tests, we cannot use the _normal_ `SniffNameUnitTest.#.inc` naming conventions for extra tests files, so the `getTestFiles()` method from the `AbstractSniffUnitTest` class needs to be overloaded to retrieve the test case files in a slightly different way.
Use the predefined `Tokens::$ooScopeTokens` array to automatically benefit from new OO structures being added to that list.
As `STDIN` won't contain a usable file name, bow out and don't examine the input again. _Note: the `process()` method should really end on a `return $this->phpcsFile->numTokens;` as well as a file containing multiple classes should not get multiple contradicting errors, but that would require each test for this sniff to be in their own file, which I currently don't deem worth the effort._
As things were, the sniff would try to find the next `T_STRING`, but didn't take live coding into account, which means it would end up comparing `$tokens[false]['content']` to the filename, which would end up comparing `<?php` to the file name. This changes the sniff to use the `File::getDeclarationName()` method instead and verifies we have a usable name to compare against before throwing the error. Includes test.
As things were, the `Squiz.Classes.ClassFileName` sniff would recommend for a class (OO structure) to be called the same as the file. However, file names can contain characters which are not allowed in PHP class names, so this advise could end up being impossible to action for names not complying with the PHP requirements for symbol names. This commit changes the error message to advise to change the file name instead of the class name, which should make the error actionable in all cases. Includes tests. While the tests would not _fail_ without this change, the tests can be used to verify the difference in the error messages on the command-line. Before this change, the errors would read like so: ``` FILE: src/Standards/Squiz/Tests/Classes/ClassFileName Spaces In FilenameUnitTest.inc ------------------------------------------------------------------------------------------------------------------------------------------------ FOUND 1 ERROR AFFECTING 1 LINE ------------------------------------------------------------------------------------------------------------------------------------------------ 7 | ERROR | Class name doesn't match filename; expected "class ClassFileName Spaces In FilenameUnitTest" (Squiz.Classes.ClassFileName.NoMatch) ------------------------------------------------------------------------------------------------------------------------------------------------ FILE: src/Standards/Squiz/Tests/Classes/ClassFileName-Dashes-In-FilenameUnitTest.inc ------------------------------------------------------------------------------------------------------------------------------------------------ FOUND 1 ERROR AFFECTING 1 LINE ------------------------------------------------------------------------------------------------------------------------------------------------ 7 | ERROR | Class name doesn't match filename; expected "class ClassFileName-Dashes-In-FilenameUnitTest" (Squiz.Classes.ClassFileName.NoMatch) ------------------------------------------------------------------------------------------------------------------------------------------------ ``` With this change, the errors will read: ``` FILE: src/Standards/Squiz/Tests/Classes/ClassFileName Spaces In FilenameUnitTest.inc ----------------------------------------------------------------------------------------------------------------------------------------------------- FOUND 1 ERROR AFFECTING 1 LINE ----------------------------------------------------------------------------------------------------------------------------------------------------- 7 | ERROR | Filename doesn't match class name; expected file name "ClassFileNameSpacesInFilenameUnitTest.inc" (Squiz.Classes.ClassFileName.NoMatch) ----------------------------------------------------------------------------------------------------------------------------------------------------- FILE: src/Standards/Squiz/Tests/Classes/ClassFileName-Dashes-In-FilenameUnitTest.inc ----------------------------------------------------------------------------------------------------------------------------------------------------- FOUND 1 ERROR AFFECTING 1 LINE ----------------------------------------------------------------------------------------------------------------------------------------------------- 7 | ERROR | Filename doesn't match class name; expected file name "ClassFileNameDashesInFilenameUnitTest.inc" (Squiz.Classes.ClassFileName.NoMatch) ----------------------------------------------------------------------------------------------------------------------------------------------------- ``` Side-note: while the `strrpos($fullPath, '.')` _could_ return `false`, this is not something we need to be concerned about in this sniff as PHPCS doesn't examine files without a file extension (at this time), so there should always be a `.` in the file name.
jrfnl
force-pushed
the
feature/squiz-classfilename-various-improvements
branch
from
c1cf608 to
cf5d4c1
Compare
braindawg
added a commit
to braindawg/PHP_CodeSniffer
that referenced
this pull request
Mar 7, 2025
Per PR PHPCSStandards#845, the filename must be modified to match the class name, since class names are much more restrictive than filenames and the inverse is not always possible. This clarifies in the example titles that the fix for the invalid code is to modify the filename rather than the class name.
jrfnl
pushed a commit
that referenced
this pull request
Mar 7, 2025
* Add XML doc for Squiz\Classes\ClassFileName sniff * [Doc] Emphasize sniff focus for ClassFileName Adjusts the `<em>` tags in the Squiz.Classes.ClassFileName sniff XML docs to better highlight the parts of the code samples that illustrate the purpose of the sniff. * Correct file/class name relation in sniff doc Per PR #845, the filename must be modified to match the class name, since class names are much more restrictive than filenames and the inverse is not always possible. This clarifies in the example titles that the fix for the invalid code is to modify the filename rather than the class name.
jrfnl
pushed a commit
that referenced
this pull request
Mar 7, 2025
* Add XML doc for Squiz\Classes\ClassFileName sniff * [Doc] Emphasize sniff focus for ClassFileName Adjusts the `<em>` tags in the Squiz.Classes.ClassFileName sniff XML docs to better highlight the parts of the code samples that illustrate the purpose of the sniff. * Correct file/class name relation in sniff doc Per PR #845, the filename must be modified to match the class name, since class names are much more restrictive than filenames and the inverse is not always possible. This clarifies in the example titles that the fix for the invalid code is to modify the filename rather than the class name.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Squiz/ClassFileName: expand the tests
This commit adds some extra tests for the sniff.
As this sniff looks at the file name of the file containing the tests, we cannot use the normal
SniffNameUnitTest.#.incnaming conventions for extra tests files, so thegetTestFiles()method from theAbstractSniffUnitTestclass needs to be overloaded to retrieve the test case files in a slightly different way.Squiz/ClassFileName: minor stability tweak
Use the predefined
Tokens::$ooScopeTokensarray to automatically benefit from new OO structures being added to that list.Squiz/ClassFileName: bow out earlier for STDIN
As
STDINwon't contain a usable file name, bow out and don't examine the input again.Note: the
process()method should really end on areturn $this->phpcsFile->numTokens;as well as a file containing multiple classes should not get multiple contradicting errors, but that would require each test for this sniff to be in their own file, which I currently don't deem worth the effort.Squiz/ClassFileName: don't error when there is no class name
As things were, the sniff would try to find the next
T_STRING, but didn't take live coding into account, which means it would end up comparing$tokens[false]['content']to the filename, which would end up comparing<?phpto the file name.This changes the sniff to use the
File::getDeclarationName()method instead and verifies we have a usable name to compare against before throwing the error.Includes test.
Squiz/ClassFileName: inverse the error message
As things were, the
Squiz.Classes.ClassFileNamesniff would recommend for a class (OO structure) to be called the same as the file.However, file names can contain characters which are not allowed in PHP class names, so this advise could end up being impossible to action for names not complying with the PHP requirements for symbol names.
This commit changes the error message to advise to change the file name instead of the class name, which should make the error actionable in all cases.
Includes tests.
While the tests would not fail without this change, the tests can be used to verify the difference in the error messages on the command-line.
Before this change, the errors would read like so:
With this change, the errors will read:
Side-note: while the
strrpos($fullPath, '.')could returnfalse, this is not something we need to be concerned about in this sniff as PHPCS doesn't examine files without a file extension (at this time), so there should always be a.in the file name.🆕 Squiz/ClassFileName: use more meaningful variable names
Suggested changelog entry
Changed: Squiz.Classes.ClassFileName: recommend changing the file name instead of changing the class name to prevent recommendations which are inactionable due to the file name not translating to a valid PHP symbol name
Fixed: Squiz.Classes.ClassFileName: don't throw an incorrect error for an unfinished OO declaration during live coding
Related issues/external references
Inspired by #843
Types of changes